Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance mode #1014

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Performance mode #1014

merged 4 commits into from
Jul 18, 2024

Conversation

JordiManyer
Copy link
Member

Hi everyone!

For some context:

The Julia flag --check-bounds=no is, as far as I understand, currently unusable due to it causing type instabilities within the compiler. Within Gridap, we use this option to enable/disable the @check macro. This means we do not currently have an option to deactivate such checks. Some heavy calculations are hidden behind those checks, so it would be nice to have the option to deactivate them.

Moreover, there are currently packages that statically (at compile time) address this issue. In particular, the Preferences.jl package offers an unified approach to deal with this. Very prominent Julia packages have been moving towards this approach, for instance MPI.jl through MPIPreferences.jl.

Given these two last points, I propose the following PR. It introduces a constant variable execution_mode that can be set to either debug (default) or performance. The latter deactivates the @check macro at compile time.

As a side note, I think we should also move towards this approach within GridapPETSc.jl and GridapP4est.jl, instead of depending on environment variables (again, the approach taken by MPI.jl).

I would appreciate some feedback on the issue, to make sure we are all on the same page.
Thanks!

@JordiManyer JordiManyer added the enhancement New feature or request label Jul 12, 2024
@JordiManyer JordiManyer self-assigned this Jul 12, 2024
@amartinhuertas
Copy link
Member

The Julia flag --check-bounds=no is, as far as I understand, currently unusable due to it causing type instabilities within the compiler.

Can you please share a reference on this? For my understanding ... Thanks!

@JordiManyer
Copy link
Member Author

The Julia flag --check-bounds=no is, as far as I understand, currently unusable due to it causing type instabilities within the compiler.

Can you please share a reference on this? For my understanding ... Thanks!

JuliaLang/julia#48245

@amartinhuertas
Copy link
Member

For the records, the PR is mostly based on the following doc page https://juliapackaging.github.io/Preferences.jl/stable/#API

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.16%. Comparing base (8749df2) to head (778678d).

Files Patch % Lines
src/Adaptivity/OldToNewFields.jl 0.00% 2 Missing ⚠️
src/Adaptivity/EdgeBasedRefinement.jl 0.00% 1 Missing ⚠️
src/CellData/CellDofs.jl 50.00% 1 Missing ⚠️
src/Helpers/Preferences.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   88.17%   88.16%   -0.01%     
==========================================
  Files         178      179       +1     
  Lines       22562    22569       +7     
==========================================
+ Hits        19895    19899       +4     
- Misses       2667     2670       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fverdugo
Copy link
Member

I like the switch based on Preferences.jl, which provably makes sense even beyond the issue with --check-bounds.

But this is only a solution to part of the problem unfortunately.

In Gridap (and multitude of Julia packages) there are a lot of loops without @inbounds, which will have bounds checks if Julia removes --check-bounds=no .... I have been reading the issue JuliaLang/julia#48245 and I don't find any clear solution at the moment apart from populating the packages with @inbounds.

Do you have any idea/opinion on how to proceed regarding array indexing if julia removes --check-bounds?

Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.

@JordiManyer
Copy link
Member Author

JordiManyer commented Jul 16, 2024

In Gridap (and multitude of Julia packages) there are a lot of loops without @inbounds, which will have bounds checks if Julia removes --check-bounds=no .... I have been reading the issue JuliaLang/julia#48245 and I don't find any clear solution at the moment apart from populating the packages with @inbounds.

Do you have any idea/opinion on how to proceed regarding array indexing if julia removes --check-bounds?

Following Tim Besard's comments on that issue, I believe the GPU people have just gone ahead and created their own compiler (which we obviously cannot do...).

Another thing coming with Julia 1.11 is the new Memory type (see this) which is supposed to be even lower-level than Array. The idea (I think) is to reduce some of the overhead in the case where all the fancy stuff from Array is not needed. I do not know if one of the things it removes is bounds-checking, but we could look into it.

if julia removes --check-bounds?

I believe the problem is that even if it does not get deprecated, it is currently worse to disable bounds checking than to just have it...

@JordiManyer JordiManyer merged commit 66cc6a4 into master Jul 18, 2024
9 checks passed
@JordiManyer JordiManyer deleted the preferences branch July 18, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants